Add temperature unit to climate and water_heater#1600
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 16 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds temperature unit support to climate and water heater entities by introducing a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
aioesphomeapi/model.py (1)
742-745: Consider defaultingtemperature_unittoTemperatureUnit.CELSIUSinstead ofNonefor consistency.Every other
APIIntEnumfield in this module uses the proto zero-value as its default (e.g.,entity_category=EntityCategory.NONE,state_class=SensorStateClass.NONE,mode=NumberMode.AUTO,port_type=SerialProxyPortType.TTL). Since protobuf will always deliver0(CELSIUS) on the wire when the sender leaves the field unset,from_pbwill never actually produceNonehere — onlyfrom_dictwith a missing key will. Usingdefault=TemperatureUnit.CELSIUSwould:
- match the documented compatibility semantics ("unset ⇒ treated as Celsius"),
- keep behavior identical between
from_pbandfrom_dict({}),- stay consistent with the surrounding enum-field pattern.
Proposed change
- temperature_unit: TemperatureUnit | None = converter_field( - default=None, - converter=TemperatureUnit.convert, - ) + temperature_unit: TemperatureUnit | None = converter_field( + default=TemperatureUnit.CELSIUS, + converter=TemperatureUnit.convert, + )(Applies to both
ClimateInfoandWaterHeaterInfo.)Also applies to: 1196-1199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aioesphomeapi/model.py` around lines 742 - 745, Change the enum field default for temperature_unit from None to TemperatureUnit.CELSIUS: update the converter_field call for the temperature_unit attribute (used in ClimateInfo and WaterHeaterInfo) to use default=TemperatureUnit.CELSIUS instead of default=None so it matches other APIIntEnum defaults, aligns from_pb/from_dict behavior, and preserves converter=TemperatureUnit.convert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@aioesphomeapi/model.py`:
- Around line 742-745: Change the enum field default for temperature_unit from
None to TemperatureUnit.CELSIUS: update the converter_field call for the
temperature_unit attribute (used in ClimateInfo and WaterHeaterInfo) to use
default=TemperatureUnit.CELSIUS instead of default=None so it matches other
APIIntEnum defaults, aligns from_pb/from_dict behavior, and preserves
converter=TemperatureUnit.convert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e2e695e-4d59-44b3-9b95-ea8f85f18eb2
📒 Files selected for processing (3)
aioesphomeapi/api.protoaioesphomeapi/api_pb2.pyaioesphomeapi/model.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1600 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 25 25
Lines 4002 4008 +6
=========================================
+ Hits 4002 4008 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a temperature unit enum and surfaces it on climate and water_heater entity info messages/models, enabling clients (notably Home Assistant) to interpret and preserve Fahrenheit-native setpoints end-to-end.
Changes:
- Introduces
TemperatureUnitin the API model layer and exposes it viaClimateInfo.temperature_unitandWaterHeaterInfo.temperature_unit. - Extends
api.prototo include theTemperatureUnitenum and addstemperature_unitfields to the relevantListEntities*Responsemessages. - Regenerates
api_pb2.pyto reflect the updated protobuf schema.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
aioesphomeapi/model.py |
Adds TemperatureUnit enum and new temperature_unit fields to climate/water-heater info dataclasses. |
aioesphomeapi/api.proto |
Defines TemperatureUnit and wires it into climate/water-heater entity info messages. |
aioesphomeapi/api_pb2.py |
Regenerated protobuf bindings reflecting the schema changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bdraco
changed the title
… downstream consumers
What does this implement/fix?
Exposes temperature unit for climate and water_heater, with corresponding changes in home-assistant/core, aioesphome, esphome, and esphome-docs, to enable native Fahrenheit measurements on those entities. Usecase is to enable accurate temperature settings on a Fahrenheit hot tub, controlled via RS485-UART. Rather than round tripped from F -> C -> C, with various floating point errors and weirdnesses, this just makes it stay F the entire way and work much more reliably.
Tests still need to be added where appropriate - that's on the todo - but the code has been validated.
When esphome is updated, but core is not, as long as you do not set the temperature unit on a climate/water heater, everything works the same as it currently is. If you do set it, you will convert from F -> "F", as home assistant will think the incoming value from ESPHome is C, without reading the UOM. If ESPHome does not have F specified, it continues to default to C.
When HA is updated, but ESPHome is on an old version, it maintains legacy behavior of treating all climate/water_heaters as using C.
Types of changes
Related issue or feature (if applicable):
Pull request in esphome (if applicable):
home-assistant/core#168261
#1586
esphome/esphome#15815
esphome/esphome-docs#6463
Checklist:
tests/folder).